Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Parquet-Exporter helm chart #195

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

cklingspor
Copy link

Moved from opencost/opencost-parquet-exporter#10

Hey everyone,

I've created a first draft of a helm chart for the parquet-exporter. I do not have a setup where I can use AWS as of now, therefore the actual push to a S3 bucket is not yet tested. However, I was able to install the chart and run the cronjob in my minikube.

Used helm-docs to create a rough documentation around the chart
The chart itself is relatively straight forward I think
Let me know what you think!

@@ -0,0 +1,24 @@
apiVersion: v2
name: parquet-exporter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we make this "opencost-parquet-exporter"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will have a ripple effect in the templates, but I prefer the verbosity/specificity

@@ -0,0 +1,24 @@
apiVersion: v2
name: parquet-exporter
description: A Helm chart for Kubernetes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"OpenCost Parquet Exporter"

@@ -0,0 +1,38 @@
# parquet-exporter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make the title and the path charts/opencost-parquet-exporter to match the repository?


| Key | Type | Default | Description |
|-----|------|---------|-------------|
| activeDeadlineSeconds | int | `3600` | Keep job runnig (from start time) for [activeDeadlineSeconds] |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"running"

- name: OPENCOST_PARQUET_S3_REGION
value: YOUR_S3_BUCKET_REGION_NAME_CHANGE_ME
- name: OPENCOST_PARQUET_SVC_HOSTNAME
value: opencost.opencost.svc.cluster.local.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there supposed to be a trailing "."?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. The dot at the end means the dns search-domain will not be appended and this is a fully qualified hostname.

@@ -0,0 +1,24 @@
apiVersion: v2
name: parquet-exporter
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will have a ripple effect in the templates, but I prefer the verbosity/specificity

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add new line at the end

ttlSecondsAfterFinished: 14400
# -- ServiceAccount use to run this pod
serviceAccount:
name: opencost
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the exporter depend on the opencost service account? I couldn't find something related to this in the exporters repo README.md.

Copy link
Author

@cklingspor cklingspor Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took that over from @lmello 's code. I have not looked into the actual requirements for the AWS S3 integration but I have to assume that its there for this purpose. Also it is not required to interact with the opencost application.
The service account key/value pair is optional however. Would you rather have me remove it from the values yaml or adjust the description to state that its an optional parameter`

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the service account is not required, I would leave it empty. Maybe also change the key to existingServiceAccount? Otherwise I would assume that it creates the service account by itself.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Ill change accordingly!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add new line at the end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add new line at the end

Copy link
Contributor

@asdfgugus asdfgugus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not test it on my cluster, but the code LGTM.

Copy link
Collaborator

@mattray mattray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the values need to move to opencost-parquet-exporter, but I didn't test.

{{/*
Common labels
*/}}
{{- define "parquet-exporter.annotations" -}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do the "parquet-exporter" references need to be changed to "opencost-parquest-exporter" across the board?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be consistent with actual chart name. I'll update accordingly

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link

This pull request has been marked as stale because it has been open for 60 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label Jun 16, 2024
@mattray mattray removed the Stale label Jun 18, 2024
Copy link

This pull request has been marked as stale because it has been open for 60 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label Aug 18, 2024
@lmello lmello added enhancement New feature or request and removed Stale labels Aug 21, 2024
@lmello
Copy link
Member

lmello commented Aug 21, 2024

I will review this PR after the change to the parquet export repo is merged.
we cannot merge this PR before this is done.

Copy link
Member

@lmello lmello left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the trailling whitespaces:
Error: 1:56 [trailing-spaces] trailing spaces
Error: 8:22 [trailing-spaces] trailing spaces
Error: 12:68 [trailing-spaces] trailing spaces
Error: 27:12 [trailing-spaces] trailing spaces
Error: 59:209 [trailing-spaces] trailing spaces
Error: 62:117 [trailing-spaces] trailing spaces

Copy link
Contributor

@brito-rafa brito-rafa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

This pull request has been marked as stale because it has been open for 60 days with no activity. Please remove the stale label or comment or this pull request will be closed in 5 days.

@github-actions github-actions bot added the Stale label Oct 27, 2024
@github-actions github-actions bot removed the Stale label Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants